Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rate limit Widget use of GiveOrder* #4301

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

esainane
Copy link
Contributor

This adds a system which poisons Spring.GiveOrder*, storing a sliding
window of command volume over time. The rate limit is provisionally set to
110c/s over a 15 second average, and can be bike shedded later.
This is to reflect the normal nature of command volume being bursty,
and sustained regular volume being troublesome.

All functions within a Widget affect the same counter.

Widgets that legitimately create large volumes of commands can be
whitelisted. The whitelist is currently in cawidgets.lua, but could
be moved to its own file if need be.

The current whitelist is certainly incomplete. In particular, area
command splitting should be whitelisted.

As a side effect, each poisoned widget receives its own local copy of
the Spring table, and modifications made to this table no longer
affect other Spring instances. Changes made to the Spring table
before widget creation, such as in cache.lua, or in Spring.Utilities,
are still shared and reflected across all widgets.

The expectation is that BLOCK_MODE=2 is the norm in a multiplayer setting.
BLOCK_MODE defaults to 1 in the absence of any instructions from infra to
permit destructive local testing, while still sending warnings about any
excessive command volume when it occurs.

This adds a system which poisons Spring.GiveOrder*, storing a sliding
window of command volume over time. The rate limit is provisionally set to
110c/s over a 15 second average, and can be bike shedded later.
This is to reflect the normal nature of command volume being bursty,
and sustained regular volume being troublesome.

All functions within a Widget affect the same counter.

Widgets that legitimately create large volumes of commands can be
whitelisted. The whitelist is currently in cawidgets.lua, but could
be moved to its own file if need be.

The current whitelist is certainly incomplete. In particular, area
command splitting should be whitelisted.

As a side effect, each poisoned widget receives its own local copy of
the Spring table, and modifications made to this table no longer
affect other Spring instances. Changes made to the Spring table
before widget creation, such as in cache.lua, or in Spring.Utilities,
are still shared and reflected across all widgets.

The expectation is that BLOCK_MODE=2 is the norm in a multiplayer setting.
BLOCK_MODE defaults to 1 in the absence of any instructions from infra to
permit destructive local testing, while still sending warnings about any
excessive command volume when it occurs.
@GoogleFrog
Copy link
Contributor

What is the performance like? Maybe disable the throttling for luauis that don't have local widgets enabled, to avoid impacting the bystanders if there is a cost.

@esainane
Copy link
Contributor Author

Theoretically, there are no temporary tables, no additional callouts (thanks to the use of currentGameFrame as an upvalue), and the overwhelming majority of bookkeeping is skipped with an early and cheap condition that only activates once per second.

Experimentally, I could not create any observable performance decrease. This held even when calibrating with huge groups of units to avoid false positives on Widgets that can send briefly send large quantities of commands, primarily Scout Dodge.

Ironically, this was a fair bit of optimization for functions that are now emphatically not meant to be called often.

@GoogleFrog
Copy link
Contributor

Sounds good.

LuaUI/cawidgets.lua Outdated Show resolved Hide resolved
@GoogleFrog
Copy link
Contributor

GoogleFrog commented Feb 19, 2021

How about this:

  • Automatically whitelist all modside widgets (the widget list knows which widgets are modside and which are local, so this must be possible).
  • Make cheat mode ignore the filter.
  • Default to blocking (maybe even disabling) widgets that hit the limit.
  • Add a little more leeway to the limit.

I don't expect to bikeshed the rate or any other form of whitelist prior to pulling.

@esainane
Copy link
Contributor Author

I'm not sure that modside widgets are safe to the point that they should all be excluded from a virtually-free rate check? It's possibly worth running with BLOCK_MODE left at 1 for a while, so we can see if anything complains without needing to worry about enforcement.

Is there a callin for when cheats are enabled? It would be nice to avoid callouts if possible.

The limit has been given a lot more leeway (exceed 110aps over 15s) than the one originally discussed and tested with (exceed 80aps over 10s)

@GoogleFrog
Copy link
Contributor

I'm not sure that modside widgets are safe to the point that they should all be excluded from a virtually-free rate check? It's possibly worth running with BLOCK_MODE left at 1 for a while, so we can see if anything complains without needing to worry about enforcement.

Gameside widgets reside at a lower customisation level so I don't want to do anything that breaks them. If they break then they should be fixed gameside. Detection without blocking achieves this.

Is there a callin for when cheats are enabled? It would be nice to avoid callouts if possible.

Not that I know of. The engine would know. You could cache it every second or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants